OCPBUGS-98465: prevent DS crash in pull secret verifier and add propagation diagnostics - #8991
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
/test e2e-aws |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe end-to-end pull-secret test now verifies propagation through hosted-control-plane and guest namespaces in separate polling stages. Kubelet verification uses a DaemonSet readiness probe to compare MD5 hashes between the node-mounted kubelet pull-secret and cluster pull-secret, then waits for all verifier pods to become Ready. Sequence Diagram(s)sequenceDiagram
participant ManagementCluster
participant HostedControlPlane
participant GuestCluster
participant VerifierDaemonSet
participant KubeletNode
ManagementCluster->>HostedControlPlane: Sync dummy pull-secret entry
HostedControlPlane-->>GuestCluster: Propagate pull-secret entry
GuestCluster->>GuestCluster: Verify pull-secret resources
VerifierDaemonSet->>KubeletNode: Run readiness-probe MD5 comparison
KubeletNode-->>VerifierDaemonSet: Report hash match
VerifierDaemonSet-->>GuestCluster: Become Ready
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
area/testing
openshift-ci
Bot
added
approved
|
/hold until we find the underneath issue which is causing the flake. |
openshift-ci
Bot
added
the
do-not-merge/hold
jparrill
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-98465, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/util/util.go (2)
2083-2083: 🧹 Nitpick | 🔵 TrivialWorst-case runtime for this subtest jumps meaningfully.
Combined, the two sequential
Eventuallycalls can now take up to 5 minutes each (10 minutes total) versus the prior 150s-based timeouts, on top of the rest of the already-longEnsureGlobalPullSecretflow. Since this change is explicitly diagnostic-only (per PR objective) and not a fix for the underlying flake, worth confirming the team is comfortable with the added CI time budget this could add on every run of this e2e test, especially if it needs to hit the full timeout repeatedly across retries.Also applies to: 2094-2094
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/util/util.go` at line 2083, Reduce the timeouts for the sequential Eventually calls in EnsureGlobalPullSecret, including the assertion for openshift-config/pull-secret propagation, so their combined worst-case duration does not substantially increase the e2e test runtime. Preserve the diagnostic behavior and existing polling interval while restoring the prior overall timeout budget.
2061-2094: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated poll-and-check pattern into a helper.
The
Get+bytes.Contains(..., []byte("e2e-dummy.example.com"))pattern is repeated three times (CP namespace secret, guestopenshift-config/pull-secret, guestkube-system/original-pull-secret) with the dummy marker string duplicated as a literal in each spot. A small shared helper would reduce duplication and centralize the marker string, lowering the risk of a typo diverging one check from the others.♻️ Suggested helper extraction
const dummyPullSecretAuthKey = "e2e-dummy.example.com" func hasDummyEntry(data []byte) bool { return bytes.Contains(data, []byte(dummyPullSecretAuthKey)) }Then reuse
hasDummyEntry(...)at lines 2072, 2082, and 2094, and referencedummyPullSecretAuthKeyat line 2053.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/util/util.go` around lines 2061 - 2094, Extract the repeated dummy pull-secret check into a shared helper near the surrounding test utilities, centralizing the marker in a named constant. Update the CP namespace check and both guest secret checks in the existing Eventually callbacks to call the helper, and use the constant wherever the dummy marker is logged or referenced, including the setup at the earlier marker location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/util/util.go`:
- Line 2083: Reduce the timeouts for the sequential Eventually calls in
EnsureGlobalPullSecret, including the assertion for openshift-config/pull-secret
propagation, so their combined worst-case duration does not substantially
increase the e2e test runtime. Preserve the diagnostic behavior and existing
polling interval while restoring the prior overall timeout budget.
- Around line 2061-2094: Extract the repeated dummy pull-secret check into a
shared helper near the surrounding test utilities, centralizing the marker in a
named constant. Update the CP namespace check and both guest secret checks in
the existing Eventually callbacks to call the helper, and use the constant
wherever the dummy marker is logged or referenced, including the setup at the
earlier marker location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 65f3f336-80b8-4a76-a1f5-e55328cd8c35
📒 Files selected for processing (1)
test/e2e/util/util.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8991 +/- ##
=======================================
Coverage 44.44% 44.44%
=======================================
Files 774 774
Lines 96977 96977
=======================================
Hits 43105 43105
Misses 50897 50897
Partials 2975 2975
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Test Resultse2e-aws
Failed TestsTotal failed tests: 5
e2e-aks
Failed TestsTotal failed tests: 4
|
|
/test e2e-aws |
|
Trying to reproduce the propagation flake issue... |
|
I have all the evidence. The analysis is clear. Here is the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe sole failing test Root CauseAWS EC2 API rate-limiting (RequestLimitExceeded) on the shared CI account. The HyperShift e2e suite runs ~20 tests in parallel, each creating its own HostedCluster with dedicated AWS infrastructure (VPCs, subnets, endpoints, etc.). At the moment The infrastructure creation sequence for This failure is not related to PR #8991. The PR modifies only Recommendations
Evidence
|
openshift-ci
Bot
added
the
needs-rebase
jparrill
changed the title
openshift-ci-robot
removed
the
jira/valid-reference
|
@jparrill: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/valid-bug
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/util/globalps.go`:
- Around line 34-37: Update CreateKubeletConfigVerifierDaemonSet to delete both
the existing verifier DaemonSet and the pull-secret before recreating them. Wait
until each deletion returns NotFound before proceeding with creation, ensuring
old pods and Secret data cannot race or persist into the new verifier.
- Around line 205-218: Update the shell command in the
wait.PollUntilContextTimeout callback to validate each md5sum invocation before
comparing hashes. Preserve the existing node and cluster hash output, but
capture or otherwise check both md5sum exit statuses so any failed command
causes the poll attempt to fail rather than allowing empty hashes to compare
equal.
In `@test/e2e/util/util.go`:
- Around line 2117-2122: Move the “Check if the on-disk kubelet config.json
matches the cluster pull secret” subtest and its
VerifyKubeletConfigWithDaemonSet call inside the pull-secret mutation subtest,
before its t.Cleanup restoration runs. Ensure it is skipped together with the
mutation when CPOAtLeast does not apply, and keep cleanup after the
verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 27a86c26-5724-48bd-9707-ecdaa5bd8fc8
📒 Files selected for processing (2)
test/e2e/util/globalps.gotest/e2e/util/util.go
| // CreateKubeletConfigVerifierDaemonSet creates a DaemonSet that mounts the | ||
| // kubelet config directory on each node. The pod stays running so the test | ||
| // can exec into it to compare the on-disk pull secret against the cluster's. | ||
| // Stale resources from a previous failed run are cleaned up before creation. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files test/e2e/util/globalps.go
wc -l test/e2e/util/globalps.go
sed -n '1,240p' test/e2e/util/globalps.goRepository: openshift/hypershift
Length of output: 8887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "CreateKubeletConfigVerifierDaemonSet|KubeletConfigVerifierNamespace|pull-secret" test/e2e/util/globalps.go
printf '\n---\n'
rg -n "wait.*NotFound|IsNotFound|Delete\\(ctx, .*DaemonSet|Delete\\(ctx, .*Secret" test/e2e/util -g '*.go'Repository: openshift/hypershift
Length of output: 3507
Delete stale verifier resources before recreating them. Delete both the DaemonSet and pull-secret first, then wait for NotFound; otherwise the recreate can race the old DaemonSet’s termination and a leftover Secret can keep stale pull-secret bytes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/util/globalps.go` around lines 34 - 37, Update
CreateKubeletConfigVerifierDaemonSet to delete both the existing verifier
DaemonSet and the pull-secret before recreating them. Wait until each deletion
returns NotFound before proceeding with creation, ensuring old pods and Secret
data cannot race or persist into the new verifier.
|
Rebased with fixed Konflux pipelines |
|
Root cause confirmed with @csrwng: The ignition server rewrites the pull secret as compact JSON when bootstrapping the node. HCCO syncs Fix: |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Thanks for the updates! One suggestion on the newline handling in the verifier probe.
| `node=$(tr -d '\n' < %s 2>/dev/null | md5sum | cut -d' ' -f1 || echo UNAVAILABLE) && `+ | ||
| `cluster=$(tr -d '\n' < /etc/pull-secret/config.json 2>/dev/null | md5sum | cut -d' ' -f1 || echo UNAVAILABLE) && `+ |
There was a problem hiding this comment.
tr -d '\n' strips all newlines from the file (interior ones included), not just trailing ones. This doesn't match what the global-pull-secret-syncer does — it uses bytes.TrimRight(content, "\n") which only removes trailing newlines.
In practice tr -d happens to work because both files get the same treatment, but it could mask a genuinely corrupted file. The POSIX-equivalent of TrimRight is command substitution (which strips trailing newlines) + printf '%s' (which outputs without adding one):
| `node=$(tr -d '\n' < %s 2>/dev/null | md5sum | cut -d' ' -f1 || echo UNAVAILABLE) && `+ | |
| `cluster=$(tr -d '\n' < /etc/pull-secret/config.json 2>/dev/null | md5sum | cut -d' ' -f1 || echo UNAVAILABLE) && `+ | |
| `node=$(printf '%%s' "$(cat %s 2>/dev/null)" | md5sum | cut -d' ' -f1 || echo UNAVAILABLE) && `+ | |
| `cluster=$(printf '%%s' "$(cat /etc/pull-secret/config.json 2>/dev/null)" | md5sum | cut -d' ' -f1 || echo UNAVAILABLE) && `+ |
There was a problem hiding this comment.
Done — switched to printf "%s" "$(cat FILE)" which strips only trailing newlines (shell command substitution behavior), matching the syncer's bytes.TrimRight semantics.
| fmt.Sprintf(`test "$(tr -d '\n' < %s 2>/dev/null | md5sum | cut -d' ' -f1 || echo FAIL_NODE)" = "$(tr -d '\n' < /etc/pull-secret/config.json 2>/dev/null | md5sum | cut -d' ' -f1 || echo FAIL_CLUSTER)"`, | ||
| NodePullSecretPath)}, |
There was a problem hiding this comment.
Same here — use printf '%%s' "$(cat ...)" to match the syncer's bytes.TrimRight(content, "\n") semantics:
| fmt.Sprintf(`test "$(tr -d '\n' < %s 2>/dev/null | md5sum | cut -d' ' -f1 || echo FAIL_NODE)" = "$(tr -d '\n' < /etc/pull-secret/config.json 2>/dev/null | md5sum | cut -d' ' -f1 || echo FAIL_CLUSTER)"`, | |
| NodePullSecretPath)}, | |
| fmt.Sprintf(`test "$(printf '%%s' "$(cat %s 2>/dev/null)" | md5sum | cut -d' ' -f1 || echo FAIL_NODE)" = "$(printf '%%s' "$(cat /etc/pull-secret/config.json 2>/dev/null)" | md5sum | cut -d' ' -f1 || echo FAIL_CLUSTER)"`, |
There was a problem hiding this comment.
Done — same change applied to the readiness probe.
|
/lgtm |
|
Scheduling tests matching the |
|
/retest-required |
…ut, and trim trailing newlines in probe The kubelet-config-verifier DaemonSet had three issues: 1. It copied openshift-config/pull-secret to kube-system at creation time. When the preceding propagation subtest restored the pull secret via t.Cleanup, the snapshot became stale. Fix: mount kube-system/original-pull-secret — the same secret the global-pull-secret-syncer uses as its source. 2. When HCCO updates original-pull-secret, it recalculates the configSeed hash and triggers a syncer DS pod restart. The verifier started checking hashes while the syncer was still restarting and had not yet written the updated content to disk. Fix: wait for the syncer DS rollout to complete (all pods updated and ready) before deploying the verifier. 3. The ignition server writes /var/lib/kubelet/config.json as compact JSON while HCCO syncs original-pull-secret in the user-supplied format (often pretty-printed with trailing newlines). The syncer handles this via bytes.TrimRight (openshift#7638), but the verifier compared raw md5sums — the trailing newline caused a permanent hash mismatch. Fix: use printf '%s' "$(cat FILE)" to strip trailing newlines before hashing, matching the syncer's bytes.TrimRight semantics. Additional changes: - Move t.Cleanup for pull secret restore to the parent test so the dummy entry stays in place during the verifier subtest. - Add fallback sentinels to the readiness probe so missing files produce distinct hashes instead of empty-string false positives. - Add UpdatedNumberScheduled check to waitForDaemonSetReady to detect in-progress rollouts. - Separate the DS readiness check into its own subtest for correct failure attribution. - Add stage-by-stage diagnostic logging to the propagation wait. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@jparrill: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
/retest-required |
|
/override e2e-kubevirt-aws-ovn-reduced |
|
@jparrill: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-kubevirt-aws-ovn-reduced |
|
@jparrill: Overrode contexts on behalf of jparrill: ci/prow/e2e-kubevirt-aws-ovn-reduced DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@jparrill: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
ddd94e5
into
openshift:main
|
@jparrill: Jira Issue Verification Checks: Jira Issue OCPBUGS-98465 Jira Issue OCPBUGS-98465 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Fix included in release 5.0.0-0.nightly-2026-07-22-233611 |
6 participants
Summary
The
kubelet-config-verifierDaemonSet was timing out (0/3 ready for 20 minutes) across multiple platforms. Root cause confirmed empirically with @csrwng and @devguyio:The ignition server rewrites the pull secret as compact JSON, while HCCO syncs
original-pull-secretin whatever format the user supplied (often pretty-printed). The syncer handles this correctly — it usesbytes.TrimRight(content, "\n")before comparing (#7638). But the verifier's readiness probe compared rawmd5sumhashes without trimming, so the trailing newline from ignition caused a permanent hash mismatch even though the syncer considered them equal.Changes
tr -d '\n'beforemd5sumin both the readiness probe and the diagnostic logging loop, matching the syncer's comparison behaviororiginal-pull-secretinstead of static copy: the previous approach copiedopenshift-config/pull-secrettokube-systemat creation time, which became stale whent.Cleanuprestored the original. Mountingoriginal-pull-secretdirectly tracks the live feature statet.Cleanupto parent test: the pull secret restore now runs after all subtests complete, so the dummy entry stays in place during the verifier subtestwaitForDaemonSetRolloutensures syncer pods are fully updated and ready before deploying the verifier|| echo FAIL_NODE/|| echo FAIL_CLUSTERprevents false positives from empty strings when files are missingUpdatedNumberScheduledcheck:waitForDaemonSetReadynow detects in-progress rolloutsRoot Cause
Confirmed by @csrwng on a live cluster: the ignition server writes
/var/lib/kubelet/config.jsonas trimmed compact JSON. HCCO createskube-system/original-pull-secretfrom the user-supplied pull secret, which may be pretty-printed with trailing newlines. The syncer'sbytes.TrimRightmakes them compare equal, so it doesn't rewrite the on-disk file. The verifier's rawmd5sumsees different bytes → readiness probe never passes.Fixes
Test plan
e2e-awspresubmit passes (TestCreateCluster/Main/EnsureGlobalPullSecret)e2e-aws-4-22presubmit passese2e-akspresubmit passes